fix(textinput): correct placeholder layout constraints (px vs DIP) and no-op NaN fontSize guard#16303
Conversation
Two defects in the native placeholder path of WindowsTextInputComponentView: 1. CreatePlaceholderLayout() built LayoutConstraints (which are in DIPs) from m_imgWidth/m_imgHeight, which are *physical* pixels (frame * pointScaleFactor). The placeholder was therefore laid out in a box pointScaleFactor x too large and rendered at a different height than the typed text at display scales > 100%. Convert the constraint back to DIPs. 2. The NaN-fontSize guard computed `TextAttributes::defaultTextAttributes().fontSize;` as a discarded expression (missing assignment), so a placeholder with no explicit fontSize kept NaN instead of the default. Assign it to textAttributes.fontSize. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree [company="Facilitron"] |
|
@microsoft-github-policy-service agree company="Facilitron" |
|
Compiled into a framework source build (binary-gated) and re-probed in our production app: no regression; the NaN-fontSize guard now actually assigns (a no-fontSize placeholder renders at the default size), and placeholder-vs-typed remain pixel-comparable at this geometry. After-shot: |
|
/azp run |
|
yeah, we should get a main check-in too otherwise it wont make it into future releases. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| // LayoutConstraints are expressed in DIPs. Feeding physical px laid the | ||
| // placeholder out in a box pointScaleFactor x too large, so the placeholder was | ||
| // measured/positioned at a different height than the typed text. Convert to DIPs. | ||
| const float scale = m_layoutMetrics.pointScaleFactor != 0.0f ? m_layoutMetrics.pointScaleFactor : 1.0f; |
There was a problem hiding this comment.
I dont think we should need the != 0 protection here -- unless that is something you saw?
There was a problem hiding this comment.
You're right — no, we didn't see it hit zero; that was purely defensive around the new division. LayoutMetrics::pointScaleFactor has a default member initializer of 1.0, and EmptyLayoutMetrics only designates .frame, so even the pre-layout sentinel inherits 1.0 — there's no normal path where it's 0 here. I'll drop the guard and just divide by m_layoutMetrics.pointScaleFactor.
|
Sounds good. Happy to re-cut onto |
2 participants
Problem
The native placeholder renders at a different height/size than typed text; a placeholder with no explicit
fontSizegets a NaN size that is never repaired.Root cause
In
CreatePlaceholderLayout(): (1)LayoutConstraintsare in DIPs, but the code feedsm_imgWidth/m_imgHeight, which are physical pixels (frame * pointScaleFactor) — the placeholder is laid out in a boxpointScaleFactor× too large. (2) The NaN-fontSize guard is a no-op:TextAttributes::defaultTextAttributes().fontSize;is a discarded expression (missing assignment), so NaN is never replaced.Fix
Divide the constraint by
pointScaleFactorto get DIPs; actually assign the default intotextAttributes.fontSize.Validation (honest)
Probed in a production RNW 0.83.2 new-arch app (Facilitron FIT, Windows 11 ARM64, Debug, 250% scale) with stock deep-import TextInputs and no app shims:
Findings stated plainly: the fontSize no-op is an unambiguous code bug (discarded expression), though on 0.83.2 a no-fontSize placeholder still renders at a plausible default — so that part is correctness/robustness, not a visible-crash fix. At this probe geometry (44-DIP box, fontSize 18) placeholder and typed text measured pixel-identical glyph rows (both parked low — dominated by the
GetContentSizecentering bug, companion PR #), so the oversized constraint did not produce a visible offset in this configuration; the mismatch that motivated the fix was observed in product forms before we suppressed native placeholders app-wide. The DIP conversion mirrors howm_imgWidth/m_imgHeightare derived (verified in 0.83.2 source). Needs upstream CI + placeholder-vs-typed comparison at >100% scale across box/font sizes, with and without explicit placeholder fontSize. Authored against0.83-stable; happy to re-cut ontomain.Microsoft Reviewers: Open in CodeFlow